Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rewrite editor-preview package as extensions of editor classes #9518

Merged
merged 1 commit into from
Jun 9, 2021
Merged

Rewrite editor-preview package as extensions of editor classes #9518

merged 1 commit into from
Jun 9, 2021

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented May 25, 2021 •
edited
Loading

What it does

This PR is a more-or-less wholesale rewrite of the editor-preview package to extend and override the functionality of the EditorManager, EditorWidgetFactory, and EditorWidget rather than using a wrapper around an EditorWidget.

These issues aren't directly addressed by these changes, but I can't reproduce them with this code:

Regressions:

  • double-clicking the title of a preview editor will not pin that editor.

I think the best way to handle this problem is what I've outlined in #9511, but in the meantime, I've achieved this by using a listener on the document.

Breaking Changes:

  • Change of various return types in WidgetOpenHandler and derived classes from Promise<T | undefined> to MaybePromise<T> | undefined to facilitate synchronous checks for widget existence.

I've removed this breakage by adding methods to the WidgetManager and WidgetOpenHandler that can check for a widget's (pending) existence synchronously rather than modifying existing methods.

How to test

Basics
  1. Makes sure that the preference editor.enablePreview is true and run the Restore workbench layout command.
  2. Single-click various files in the navigator.
  3. One editor should be created, and it should be replaced with each new file opened.
  4. Double click the file you have open in the tree.
  5. That editor should switch to non-preview mode.
  6. Single click another file.
  7. A new editor preview should open up.
  8. Single click the file in the pinned (non-preview) editor.
  9. The non-preview editor should be revealed, and no new editor should be opened.
  10. Focus the current preview editor and start typing.
  11. It should turn into a normal editor.
Fixes
  • Renaming
    1. Open a preview, then rename the file.
    2. The preview with the old name should be replaced with a new one with the new name.
    3. Single-clicking a different file should replace the preview.
  • Navigating
    1. Open an editor preview in a source code file.
    2. Use context menu 'Go to references' or Shift + F12 to view a references peek widget
    3. Clicking on the destinations should work fine.
  • Dragging
    1. Open a preview.
    2. Drag it into a side panel
    3. You should still see the editor.
    4. Drag it back
    5. It should be a non-preview
  • Rapid opening
    1. Follow the reproduction instructions in Race condition in Editor Preview Manager and Editor Manager #9485
    2. You should never wind up with two editors.
Others
  • Using ctrl+w (Electron) / alt+w (Browser) to close an editor preview should work.
  • Opening a preview editor for e.g. a Markdown file and then using the Open preview should work as expected.

Review checklist

Reminder for reviewers

Signed-off-by: Colin Grant colin.grant@ericsson.com

@colin-grant-work colin-grant-work added the editor-preview issues that are related to the editor-preview label May 25, 2021
@colin-grant-work colin-grant-work changed the title Rewrite editor-preview package as extensions editor classes Rewrite editor-preview package as extensions of editor classes May 25, 2021
@colin-grant-work colin-grant-work force-pushed the bugfix/preview-editors branch 2 times, most recently from ce92832 to aacc54e Compare May 26, 2021 21:02
kenneth-marut-work
kenneth-marut-work reviewed May 26, 2021
View reviewed changes
Copy link
Contributor

@kenneth-marut-work kenneth-marut-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working much better and it seems to have fixed the issues mentioned above. On a first pass running through the functionality, I only noticed two small behavioral issues related to pinning:

  1. Splitting the editor view in the main area
  • Open 2 files (one being a preview)
  • Drag the preview to the right side of the MAIN area to split the screen
  • The preview does not pin
  1. Pinning an editor when it does not exist in the file tree
  • Open a file using the steps in Race condition in Editor Preview Manager and Editor Manager #9485 with a file that does not exist in your current workspace
  • Observe that the file is opened as a preview widget, but it is not possible to pin it easily (now that the double clicking does not pin)
  • In VSCode there is a context command for "Keep Editor Open" which might be a useful approach

@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented May 26, 2021 •
edited
Loading

@kenneth-marut-work, thanks for taking a look. I've made some changes to address your comments

  • changed the drag logic to track the widget's tabbar rather than its parent to cover dragging inside the main area.
  • added context menu commands and a keyboard shortcut parallel to VSCode's 'Keep Open' command.
  • brought back the pin-on-double-click logic with a listener on the document.

@kenneth-marut-work
Copy link
Contributor

Verified that the new context menu pin option and double clicking work 👍

@vince-fugnitto vince-fugnitto added the quality issues related to code and application quality label Jun 1, 2021
@colin-grant-work colin-grant-work force-pushed the bugfix/preview-editors branch 2 times, most recently from 684cff7 to 1e9a861 Compare June 1, 2021 19:53
vince-fugnitto
vince-fugnitto approved these changes Jun 2, 2021
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified the following functionality 👍

Preference editor.enablePreview:

  • the preference is enabled by default
  • when true editors are opened as a preview and replaced
  • when false editors are opened normally and not replaced
  • when toggling from true to false, the open preview editor is transformed into a normal editor

Application

  • a preview editor is transformed into a normal editor when it is modified by a user
  • a single-click in the explorer opens a preview editor when the preference is on
  • a single-click in the explorer opens a normal editor when the preference is off
  • a double-click in the explorer opens a normal editor (preference on)
  • renaming a preview editor works correctly - the editor remains a preview
  • dragging preview editors works correctly (sidepanel to main-area and so-on)
  • keybinding successfully closes a preview editor
  • splitting a preview editor works
  • reloading the application with preview editors works
  • performing toolbar actions (ex: 'markdown: open preview') should work for preview editors

@colin-grant-work
Copy link
Contributor Author

@caseyflynn-google, this may be of interest to you. I plan to merge on Monday or Tuesday, if there are no objections.

@kenneth-marut-work
Copy link
Contributor

Found one small issue, right clicking a file and using the "Open With" menu option reveals 2 "Code Editor" openers

image

@colin-grant-work colin-grant-work force-pushed the bugfix/preview-editors branch 2 times, most recently from 2d6fc03 to da51685 Compare June 3, 2021 20:16
@colin-grant-work
Copy link
Contributor Author

Found one small issue, right clicking a file and using the "Open With" menu option reveals 2 "Code Editor" openers

@kenneth-marut-work, good find. Since WidgetOpenHandler was already bound to the EditorManager, I didn't need to bind it to the EditorPreviewManager again. I've removed that, and now there should be 1 (or 0) 'Open With' entries.

@colin-grant-work colin-grant-work force-pushed the bugfix/preview-editors branch 2 times, most recently from 58a4adf to bf7e2c0 Compare June 7, 2021 17:11
caseyflynn-google
caseyflynn-google approved these changes Jun 8, 2021
View reviewed changes
Copy link
Contributor

@caseyflynn-google caseyflynn-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Verified all items listed in the testing section:

How to test

Basics
  1. Makes sure that the preference editor.enablePreview is true and run the Restore workbench layout command.
  2. Single-click various files in the navigator.
  3. One editor should be created, and it should be replaced with each new file opened.
  4. Double click the file you have open in the tree.
  5. That editor should switch to non-preview mode.
  6. Single click another file.
  7. A new editor preview should open up.
  8. Single click the file in the pinned (non-preview) editor.
  9. The non-preview editor should be revealed, and no new editor should be opened.
  10. Focus the current preview editor and start typing.
  11. It should turn into a normal editor.
Fixes

  • Renaming

    1. Open a preview, then rename the file.
    2. The preview with the old name should be replaced with a new one with the new name.
    3. Single-clicking a different file should replace the preview.

  • Navigating

    1. Open an editor preview in a source code file.
    2. Use context menu 'Go to references' or Shift + F12 to view a references peek widget
    3. Clicking on the destinations should work fine.

  • Dragging

    1. Open a preview.
    2. Drag it into a side panel
    3. You should still see the editor.
    4. Drag it back
    5. It should be a non-preview

  • Rapid opening

    1. Follow the reproduction instructions in Race condition in Editor Preview Manager and Editor Manager #9485
    2. You should never wind up with two editors.
Others
  • Using ctrl+w (Electron) / alt+w (Browser) to close an editor preview should work.
  • Opening a preview editor for e.g. a Markdown file and then using the Open preview should work as expected.

Additional checks:

  • All context menu items in editor preview tab work (Close, Close Others, Toggle Maximized etc)
  • Language server features work in editor preview
  • Editor reviews are properly revived (only tested in browser)

@colin-grant-work colin-grant-work force-pushed the bugfix/preview-editors branch 2 times, most recently from d5cd762 to e70dfb8 Compare June 9, 2021 14:35
@colin-grant-work colin-grant-work force-pushed the bugfix/preview-editors branch 3 times, most recently from 8393b22 to 19049a7 Compare June 9, 2021 14:55
@colin-grant-work
Rewrite Editor Preview as extensions of Editor classes
a349c80 
Signed-off-by: Colin Grant <colin.grant@ericsson.com>
@colin-grant-work colin-grant-work merged commit e8e88b7 into eclipse-theia:master Jun 9, 2021
@colin-grant-work colin-grant-work deleted the bugfix/preview-editors branch June 9, 2021 15:43
@colin-grant-work colin-grant-work added this to the 1.15.0 milestone Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@kenneth-marut-work kenneth-marut-work kenneth-marut-work left review comments

@caseyflynn-google caseyflynn-google caseyflynn-google approved these changes

Assignees
No one assigned
Labels
editor-preview issues that are related to the editor-preview quality issues related to code and application quality
Projects
None yet
Milestone
1.15.0
4 participants
@colin-grant-work @kenneth-marut-work @caseyflynn-google @vince-fugnitto